-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Get rid of the dotmap and generate Munches for multsigs and extras too. #72
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see implementation of self.populate_extras()
and self.populate_multisigs()
. Accessing property will literally throw an exception
ok @SHAKOTN please take another look. |
# Conflicts: # bal_addresses/__init__.py # bal_addresses/addresses.py # bal_addresses/generate_current_permissions.py # bal_addresses/permissions.py
bal_addresses/addresses.py
Outdated
self._multisigs = Munch.fromDict(msigs[self.chain]) | ||
else: | ||
print(f"Warning: No multisigs for chain {self.chain}, multisigs must be added in extras/multisig.json") | ||
self._multisigs = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it empty Munch object to unify the interface
@@ -76,10 +105,36 @@ def test_deployments_invalid_format(): | |||
} | |||
} | |||
) | |||
responses.add( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely done, but please pay attention to the name of the test.
It's always good to make tests as small as possible.
My suggestion: create separate test functions for both extras and msigs logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried that but you have to copy all the inputs each time and then there is very little additional code. It felt silly and made changes to the test inputs hard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should separate all the test cases in smaller units, that is my recommendation. It's better to NOT test everything in one function even if it leads to boilerplate code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another day ser :)
No description provided.